Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Catch warnings as errors in pytest #283

Merged
merged 2 commits into from
Aug 30, 2024
Merged

Conversation

mschoettle
Copy link
Contributor

@mschoettle mschoettle commented Mar 20, 2024

Make pytest treat warnings as errors to catch problems as early as possible. For example, Django deprecation warnings. This should help in keeping up with compatibility for future Django versions.

@mschoettle
Copy link
Contributor Author

@jheld @samamorgan I wanted to capture warnings as errors in pytest so that deprecation notices are exposed as early as possible. I noticed in our project where I tried to switch to the new alpha release that even though the index_together is fixed in the model (via #258) the old migration still adds it causing the "RemovedInDjango51" warning. I think the only way would be to change the old migration such that new users only get the new behaviour and existing users who already ran this migration on their project get the updates via the migration added in #258. Thoughts?

@samamorgan I noticed that even when the tests fail the CI job does not fail due to piping the output to tee. I fixed it by explicitly setting the shell to bash which sets set -eo pipefail. See: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#exit-codes-and-error-action-preference

Side question: What's the purpose of running ruff as part of pytest? Would this not be captured by the pre-commit job already? Aside, I would favour a separate ruff job instead of mixing it with pytest.

@mschoettle mschoettle changed the title WIP: Catch warnings as errors in pytest Draft: Catch warnings as errors in pytest Mar 20, 2024
@samamorgan
Copy link
Contributor

On the migrations, I think that just means it's time to squash the migrations. That should get rid of the warning.

Pre-commit should always run as part of CI. A user can easily turn off pre-commit and commit code that does not follow linting rules. CI is the last defense against that. I don't think it should run separately, as that would increase test time since the test environment would have to be regenerated. I'm pretty sensitive to that sort of thing since increased CI time = increased cost in a private repo.

@mschoettle
Copy link
Contributor Author

mschoettle commented Mar 20, 2024

On the migrations, I think that just means it's time to squash the migrations. That should get rid of the warning.

How does migrate behave when an existing project has applied the previous migrations (that were then squashed)?

Pre-commit should always run as part of CI. A user can easily turn off pre-commit and commit code that does not follow linting rules. CI is the last defense against that.

I agree with you that pre-commit should run as part of CI for the reason you stated.

I don't think it should run separately, as that would increase test time since the test environment would have to be regenerated. I'm pretty sensitive to that sort of thing since increased CI time = increased cost in a private repo.

I am assuming this is in relation to my question about ruff and pytest? I understand your point. To me these are two separate concerns. One is linting, the other is tests. Since ruff and ruff-format are run as part of pre-commit job it is redundant to run it again via pytest.

@samamorgan
Copy link
Contributor

samamorgan commented Mar 20, 2024

How does migrate behave when an existing project has applied the previous migrations (that were then squashed)?

https://docs.djangoproject.com/en/5.0/topics/migrations/#migration-squashing

What I believe will happen: For those who have already installed this package and run migrations, the new migration will co-exist and do nothing. For those who are installing this the first time, the squashed migration will be chosen. I think the same PR can both squash the migrations and delete the old ones. Any complexity this represents on behalf of external users of this module will have to be dealt with by those users. This is a normal, expected operation for Django. We may see users come in submitting issues that have themselves in some sort of pickle, and we can just guide them appropriately if that happens, though I suspect even that is unlikely. I trust Django's migration process.

I am assuming this is in relation to my question about ruff and pytest? I understand your point. To me these are two separate concerns. One is linting, the other is tests. Since ruff and ruff-format are run as part of pre-commit job it is redundant to run it again via pytest.

Ahh, I see what you mean. I added pytest-ruff, which I totally forgot about. I think the appropriate action is to just remove pytest-ruff completely, since pre-commit makes that redundant. That way, any expansion of pre-commit will be completely covered, and we won't have to add any more cruft to the test suite.

@samamorgan
Copy link
Contributor

samamorgan commented Mar 20, 2024

I wanted to capture warnings as errors in pytest so that deprecation notices are exposed as early as possible.

I think this may be a mistake. Tests are supposed to catch errors. A deprecation warning isn't an error, just a warning of some sort of potential future consequence. Deprecations should be handled on a case-by-case basis, and we don't want tests failing for reasons that aren't actually failures. This reduces the value of tests:

  • Tests should illustrate that the code changes made do not cause breaking changes
  • Tests failures should never become "noise": If a failure happens, it needs to be actionable inside the PR that the test was run, not require a follow-up PR to address

All that is to say, a PR should only be merged if tests pass. That should be a hard line. I would hate to see a situation where a PR is accepted with failing tests.

@mschoettle
Copy link
Contributor Author

Thanks!

Removing pytest-ruff: #284
Squashing migrations: #285

@mschoettle
Copy link
Contributor Author

mschoettle commented Mar 21, 2024

I think this may be a mistake. Tests are supposed to catch errors. A deprecation warning isn't an error, just a warning of some sort of potential future consequence. Deprecations should be handled on a case-by-case basis, and we don't want tests failing for reasons that aren't actually failures. This reduces the value of tests:

  • Tests should illustrate that the code changes made do not cause breaking changes
  • Tests failures should never become "noise": If a failure happens, it needs to be actionable inside the PR that the test was run, not require a follow-up PR to address

You raise some good points. The problem that treating warnings as errors solves is that otherwise these warnings are never visible anywhere and therefore will be missed. A lot of the warnings from this package I found out about because of treating them as errors in our project which subsequently made me do the corresponding PR here to address them.

Unless there is another way I don't know about that can make them visible without causing the problems that you outline. However, these errors should only appear if we add support for a new version of Python and/or Django which would be in its own PR.

@samamorgan
Copy link
Contributor

samamorgan commented Mar 25, 2024

@mschoettle I think I've been swayed. One scenario where this would be pretty valuable: Say a contributor adds a third-party package that itself raises Django deprecation warnings (my professional project currently has several packages in this condition). We'd immediately catch that as an error, causing tests to fail, and that gives us clear evidence to reject a contribution.

@mschoettle mschoettle marked this pull request as ready for review April 17, 2024 14:32
@mschoettle mschoettle changed the title Draft: Catch warnings as errors in pytest Catch warnings as errors in pytest Apr 17, 2024
@mschoettle
Copy link
Contributor Author

With #285 being merged I brought this PR up to date and the tests now pass (no more deprecation warnings).

@@ -61,6 +61,8 @@ jobs:
run: poetry run pre-commit run --all-files

- name: Run Tests (Django ${{ steps.poetry-install.outputs.django_version }})
# enforce failing fast: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#exit-codes-and-error-action-preference
shell: bash
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch! Just caught this myself on another project (pytest failing to initialize actually caused the job to technically succeed!)

@mschoettle
Copy link
Contributor Author

@jheld Please review

@jheld
Copy link
Collaborator

jheld commented Aug 29, 2024

Can you rebase first?

@mschoettle
Copy link
Contributor Author

@jheld Done. I also had to update the poetry lock file (it was out of date). I left that as a separate commit.

@jheld jheld merged commit 695c469 into soynatan:master Aug 30, 2024
6 checks passed
@mschoettle mschoettle deleted the pytest-errors branch August 30, 2024 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants